Add MCP server registry UI: version management, structured server.json, tools, compare view, aliases, and tags#289
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. 🗂️ Base branches to auto review (4)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Central YAML (base), Organization UI (inherited) Review profile: CHILL Plan: Enterprise Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
e89633b to
a85306a
Compare
6ba364a to
aaf82fc
Compare
a85306a to
34ac83a
Compare
ff1d3b2 to
1da37d3
Compare
7016c96 to
417418e
Compare
| wordBreak: 'break-word', | ||
| }} | ||
| > | ||
| <code>{baselineJson || 'Empty'}</code> |
There was a problem hiding this comment.
i18n (Medium): Hardcoded English string 'Empty' — should use <FormattedMessage> or intl.formatMessage() for i18n support.
<code>{baselineJson || intl.formatMessage({ defaultMessage: 'Empty', description: 'Fallback for empty server JSON in compare view' })}</code>There was a problem hiding this comment.
Valid. The Prompts compare view (PromptContentCompare.tsx line 169) also uses a hardcoded 'Empty' string. Both should use intl.formatMessage(), but since the source pattern has the same issue, this is consistent. Can be addressed as a follow-up.
| <div | ||
| css={{ | ||
| display: 'grid', | ||
| gridTemplateColumns: '100px 1fr', |
There was a problem hiding this comment.
Consistency (Medium): gridTemplateColumns: '100px 1fr' here but MCPServerVersionDetail.tsx uses '120px 1fr' for the same metadata grid pattern. These should match for visual consistency. Also consider using a shared constant so both stay in sync.
There was a problem hiding this comment.
Valid. We aligned this to 120px in our branch (PR #292) to match MCPServerVersionDetail. This PR should pick up the same value.
| <Button | ||
| componentId="mlflow.mcp_registry.detail.version.edit_display_name" | ||
| size="small" | ||
| icon={<PencilIcon />} |
There was a problem hiding this comment.
Accessibility (High): This icon-only <Button> with <PencilIcon> has no aria-label, making it invisible to screen readers. Same issue on the edit buttons for description (line ~166), status (line ~296), and metadata (line ~353).
Suggested fix — add aria-label like the tags edit button already does:
<Button
aria-label={intl.formatMessage({
defaultMessage: "Edit display name",
description: "Aria label for edit version display name button",
})}
icon={<PencilIcon />}
...
/>There was a problem hiding this comment.
Valid. Icon-only buttons must have aria-label for screen reader accessibility. We added aria-label to our delete binding button in PR #292 following the same pattern.
| onClearLatestError={() => setLatestMutation.reset()} | ||
| resolvedLatestVersion={resolvedLatestVersion} | ||
| onUpdateDescription={async (description) => { | ||
| await MCPRegistryApi.updateMCPServer(serverName, { description }); |
There was a problem hiding this comment.
UX (Medium): This calls updateMCPServer(serverName, { description }) which updates the server-level description, but the button is placed inside the version detail panel. Users may think they're editing a version-level property. Consider either:\n- Moving the description editing to the server-level header area (above the version panel)\n- Adding a visual cue/label that this is the "Server description" not "Version description"
There was a problem hiding this comment.
Valid observation. The server description vs version description distinction can be confusing in the current layout. A label like "Server description" would help. Not a blocker but worth improving.
| }, [versions, selectedVersion, viewState.comparedVersion, setComparedVersion, setSelectedVersion]); | ||
|
|
||
| useEffect(() => { | ||
| setLatestMutation.reset(); |
There was a problem hiding this comment.
Bug (Medium): This resets the mutation state when selectedVersion changes, but doesn't cancel the in-flight mutation. If a user clicks "Set as latest" then quickly switches versions, the mutation continues in the background and its result will apply to the wrong version's UI state.\n\nSuggested fix — also add a comment explaining the eslint-disable:\ntsx\nuseEffect(() => {\n // Reset mutation state when switching versions to avoid showing\n // stale error/success from a different version.\n // setLatestMutation is excluded from deps to avoid infinite loop\n // (reset() changes the mutation reference).\n setLatestMutation.reset();\n}, [selectedVersion]); // eslint-disable-line react-hooks/exhaustive-deps\n\n\nIdeally, also track which version the mutation was initiated for and only show loading/error when it matches the current selectedVersion.
There was a problem hiding this comment.
Valid. The eslint-disable comment explaining why setLatestMutation is excluded from deps is good practice. The race condition concern is real but low-risk — the mutation result is only used for UI state (loading spinner, error display) and the cache invalidation ensures data correctness regardless.
| const [editingBinding, setEditingBinding] = useState<MCPAccessBinding | undefined>(undefined); | ||
| const [deletingBinding, setDeletingBinding] = useState<MCPAccessBinding | undefined>(undefined); | ||
| const [editServerDisplayNameVisible, setEditServerDisplayNameVisible] = useState(false); | ||
| const [serverDisplayNameSaving, setServerDisplayNameSaving] = useState(false); |
There was a problem hiding this comment.
Pattern (Medium): This manually manages serverDisplayNameSaving/Error/Visible state with .then()/.catch()/.finally(), while every other mutation on this page uses React Query hooks. For consistency, consider creating a useUpdateMCPServerDisplayName hook (parallel to useUpdateMCPServerVersionDisplayName) — it would eliminate ~15 lines of manual state management and centralize cache invalidation.
There was a problem hiding this comment.
Valid. Extracting a useUpdateMCPServerDisplayName React Query hook would align with the pattern used by all other mutations on this page and eliminate manual .then()/.catch()/.finally() state management.
| } | ||
| if (viewState.comparedVersion && !versions.some((v) => v.version === viewState.comparedVersion)) { | ||
| setComparedVersion( | ||
| versions[0]?.version === selectedVersion ? (versions[1]?.version ?? '') : (versions[0]?.version ?? ''), |
There was a problem hiding this comment.
Bug (Low): When only 1 version remains after deletion, versions[1]?.version ?? '' sets comparedVersion to an empty string. This causes the compare view to render a half-empty panel (no version found for '').\n\nSuggested fix: switch back to preview mode when fewer than 2 versions remain:\ntsx\nif (viewState.comparedVersion && !versions.some((v) => v.version === viewState.comparedVersion)) {\n if (versions.length < 2) {\n setPreviewMode();\n } else {\n setComparedVersion(\n versions[0]?.version === selectedVersion ? versions[1]?.version : versions[0]?.version,\n );\n }\n}\n
There was a problem hiding this comment.
Valid. Switching back to preview mode when fewer than 2 versions remain is the correct behavior. The suggested fix is clean.
| }, | ||
| onSuccess: (_data, { serverJson }) => { | ||
| const name = serverJson.name; | ||
| queryClient.invalidateQueries(['mcp_servers_list']); |
There was a problem hiding this comment.
Consistency (Medium): These use hardcoded query key strings ('mcp_server', 'mcp_servers_list', etc.) instead of MCP_QUERY_KEYS constants from utils.ts. The existing useInvalidateServerQueries helper and query hooks all use the constants. Also, 'mcp_server_latest_version' has no constant entry in MCP_QUERY_KEYS at all.\n\nSuggested fix:\n1. Add LATEST_VERSION: 'mcp_server_latest_version' to MCP_QUERY_KEYS in utils.ts\n2. Replace all raw strings here with MCP_QUERY_KEYS.* constants\n3. Better yet, use the shared useInvalidateServerQueries() helper instead of manual invalidation
There was a problem hiding this comment.
Valid. We extracted MCP_QUERY_KEYS constants in PR #292 and moved all existing hooks to use them. New hooks in this PR should follow the same pattern. The useInvalidateServerQueries() helper already exists and handles the full invalidation set.
| mutationFn: ({ version, displayName }: { version: string; displayName: string }) => | ||
| MCPRegistryApi.updateMCPServerVersion(serverName, version, { display_name: displayName || null }), | ||
| onSuccess: () => { | ||
| queryClient.invalidateQueries(['mcp_server', serverName]); |
There was a problem hiding this comment.
Consistency (Medium): useUpdateMCPServerVersionDisplayName and useSetLatestVersion (below) manually call queryClient.invalidateQueries() with raw strings and only invalidate a subset of keys. The existing pattern in this same file uses useInvalidateServerQueries() which invalidates all 5 relevant keys. Missing SERVER_BINDINGS and BINDINGS_LIST invalidation could cause stale cache issues.
Suggested fix: use the shared useInvalidateServerQueries() helper for both new hooks, same as useUpdateMCPServerVersionStatus and useDeleteMCPServerVersion already do.
There was a problem hiding this comment.
Valid. The useInvalidateServerQueries() helper already exists in this file and handles all 5 keys. New mutations should use it rather than manual invalidateQueries calls with a subset of keys.
|
|
||
| const updateMutation = useMutation<unknown, Error, UpdateTagsPayload>({ | ||
| mutationFn: async ({ toAdd, toDelete, serverName }) => { | ||
| return Promise.all([ |
There was a problem hiding this comment.
Duplication (Medium): This Promise.all([...toAdd.map(set), ...toDelete.map(del)]) + useEditKeyValueTagsModal + diffCurrentAndNewTags wiring is duplicated here, in MCPRegistryPage.tsx, and in useUpdateMCPServerVersionMetadataModal.tsx.
The server-level tag editing (this component and MCPRegistryPage) is structurally identical. Consider extracting a shared useEditMCPServerTagsModal(serverName, onTagsUpdated) hook that both consumers can call, similar to how useUpdateMCPServerVersionMetadataModal encapsulates version-level tag editing.
There was a problem hiding this comment.
Valid but acceptable. The tag editing wiring is repeated but each instance has slightly different context (server-level vs version-level tags, different invalidation needs). Extracting a shared helper would require parameterizing the API calls and invalidation, which may add complexity without much gain for 3 call sites.
| export const validateServerJson = (value: string): ServerJsonValidationResult => { | ||
| const trimmed = value?.trim(); | ||
| if (!trimmed) { | ||
| return { valid: false, error: 'Server definition is required' }; |
There was a problem hiding this comment.
i18n (High): These validation error messages are hardcoded English strings that get surfaced to users via Alert components. All 9 messages in validateServerJson() and validateToolsJson() should use intl.formatMessage().
Since these are pure utility functions, you could either:
- Accept
intlas a parameter:validateServerJson(json: string, intl: IntlShape) - Return error keys (e.g.,
'server_json_required') and map to i18n messages in the component layer
There was a problem hiding this comment.
Valid but low priority. These are JSON schema validation error messages shown in developer-facing modals (create version form). The messages describe technical JSON structure issues like "server_json must have a name field". Similar validation in the codebase (e.g., form validation in admin components) uses hardcoded English. Worth fixing eventually but not a blocker — the error messages are shown inside Alert components in modals, not in the main UI flow.
| switch (action.type) { | ||
| case 'setPreviewMode': | ||
| return { ...state, mode: MCPServerDetailViewMode.PREVIEW, comparedVersion: undefined }; | ||
| case 'setCompareMode': |
There was a problem hiding this comment.
Bug (Medium): No guard prevents selectedVersion === comparedVersion. A user can click both baseline and compared radios on the same version row, resulting in a self-diff. The setSelectedVersion and setComparedVersion reducers should auto-swap when they'd create a collision:\n\ntsx\ncase 'setSelectedVersion':\n return {\n ...state,\n selectedVersion: action.version,\n // If this would collide with compared, swap them\n comparedVersion: action.version === state.comparedVersion\n ? state.selectedVersion\n : state.comparedVersion,\n };\n
There was a problem hiding this comment.
Valid. Good catch — the Prompts version avoids this because setCompareMode always picks two different versions, but a user could manually click the same row's baseline+compared buttons. The auto-swap suggestion is clean. The Prompts implementation handles this implicitly because the switchSides function swaps them, but there's no guard on direct selection.
| server.use(getMockedSearchMCPServerVersionsResponse([deletedVersion])); | ||
| renderPage(); | ||
| await waitFor(() => { | ||
| expect(screen.getByText('Viewing version 1')).toBeInTheDocument(); |
There was a problem hiding this comment.
Testing (Medium): Several new components/hooks in this PR have no dedicated test coverage:
MCPServerTagsBox.tsx— tag CRUD save flow untestedUpdateVersionDisplayNameModal.tsx— save/error flow anduseEffectreset behavior untestedMCPServerVersionDiffSelectorButton.tsx— radio accessibility attributes untesteduseUpdateMCPServerVersionMetadataModal.tsx— mutation calls and query invalidation untested
Also: the existing MCPServerDetailPage.test.tsx tests open modals and check error paths but rarely verify happy-path completion (e.g., save succeeds → modal closes → data refreshes). Consider adding at least one success-flow test per modal.
There was a problem hiding this comment.
Valid. PR #292 adds test coverage for compare components (MCPServerVersionCompare, MCPServerVersionDiffSelectorButton) and compare mode integration tests. The new hooks and modals in this PR should have matching coverage.
| ); | ||
| }); | ||
| }, | ||
| }); |
There was a problem hiding this comment.
Consistency (Low): Same issue as the create mutation — raw query key string 'mcp_server_versions' instead of MCP_QUERY_KEYS.SERVER_VERSIONS. This hook should also use the constants for consistency.
There was a problem hiding this comment.
Valid. Should use MCP_QUERY_KEYS.SERVER_VERSIONS constant. Quick fix.
| }} | ||
| /> | ||
| </ScrollablePageWrapper> | ||
| ); |
There was a problem hiding this comment.
Nit: MCPRegistryPage was updated to use the new ErrorUtils.mlflowServices.MCP_REGISTRY, but this page and MCPAccessBindingDetailPage still use ErrorUtils.mlflowServices.EXPERIMENTS. Should be updated for consistency.
There was a problem hiding this comment.
Valid. Both MCPServerDetailPage and MCPAccessBindingDetailPage should use the new ErrorUtils.mlflowServices.MCP_REGISTRY for consistency with MCPRegistryPage.
e2c2b18 to
024cbbf
Compare
| serverName: string; | ||
| toAdd: { key: string; value: string }[]; | ||
| toDelete: { key: string }[]; | ||
| }; |
There was a problem hiding this comment.
Bug (Medium): onSuccess only invalidates SERVERS_LIST but not MCP_QUERY_KEYS.SERVER for the specific server. When tags are edited from the detail page, the server object stays stale until revisited. The onTagsUpdated callback covers this on the detail page via refetchAll, but the per-server query cache remains stale.
Should also invalidate [MCP_QUERY_KEYS.SERVER, serverName], or better yet use the useInvalidateServerQueries helper.
| ); | ||
| }); | ||
| }, | ||
| }); |
There was a problem hiding this comment.
Consistency (Medium): This only invalidates SERVER_VERSIONS but not SERVER_LATEST_VERSION. Version metadata changes could affect the latest version entity. Consider using useInvalidateServerQueries() which handles all 5+ query keys, same as the other mutation hooks in this file.
| const setTag = isNewServer | ||
| ? (key: string, value: string) => MCPRegistryApi.setMCPServerTag(name, { key, value }) | ||
| : (key: string, value: string) => | ||
| MCPRegistryApi.setMCPServerVersionTag(name, version.version, { key, value }); |
There was a problem hiding this comment.
Bug (Medium): The catch {} block silently swallows all errors from the secondary operations (display name, description, tags). The user sees success and navigates to the detail page, but these fields may not have been applied. At minimum, consider showing a warning notification like "Version created but some metadata could not be saved" so the user knows to retry.
| server.use(getMockedSearchMCPServerVersionsResponse([deletedVersion])); | ||
| renderPage(); | ||
| await waitFor(() => { | ||
| expect(screen.getByText('Viewing version 1')).toBeInTheDocument(); |
There was a problem hiding this comment.
Testing (Medium): Good test coverage overall — useCreateMCPServerVersionModal (9 tests), MCPServerVersionCompare (5), MCPServerVersionDiffSelectorButton (5), useMCPServerDetailViewState (8), plus ~200 lines added to MCPServerDetailPage.test.tsx.
Still missing dedicated tests for:
useUpdateMCPServerVersionMetadataModal— no test fileMCPServerTagsBox— no test fileuseUpdateMCPServerTags— no test file
These get partial coverage via page-level integration tests, but dedicated unit tests would catch edge cases (e.g., the missing cache invalidation in useUpdateMCPServerTags).
| </div> | ||
| <div css={{ flex: 1 }}> | ||
| <VersionMetadataGrid | ||
| version={comparedVersion} |
There was a problem hiding this comment.
Duplication (Low): The JSON <pre> styling (margin: 0, padding, backgroundColor, borderRadius, overflow, fontSize, fontFamily) is duplicated between MCPServerVersionCompare (the jsonPanelStyles constant) and MCPServerVersionDetail (inline on the <pre> inside CollapsibleSection). Consider extracting into a shared jsonPanelStyles constant in utils.ts or a small JsonCodeBlock component.
f25db61 to
e5e73d0
Compare
024cbbf to
d8a640d
Compare
Signed-off-by: Nana Nosirova <10577112+nananosirova@users.noreply.github.com>
Signed-off-by: Nana Nosirova <10577112+nananosirova@users.noreply.github.com>
Signed-off-by: Nana Nosirova <10577112+nananosirova@users.noreply.github.com>
Signed-off-by: Nana Nosirova <10577112+nananosirova@users.noreply.github.com>
d8a640d to
af5a618
Compare
af5a618 to
728e57b
Compare
728e57b to
27b33e6
Compare
27b33e6 to
1750be9
Compare
|
[APPROVALNOTIFIER] This PR is APPROVED Approval requirements bypassed by manually added approval. This pull-request has been approved by: DaoDaoNoCode The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
ce19d9f
into
opendatahub-io:mcp-registry-prototype
Summary
MCP server registry UI for RHOAIENG-65775. This PR adds version management, a compare view, latest version resolution, alias management, server-level tags, and structured
server.jsonrendering.Key features:
server.jsondisplay: packages as expandable rows with env vars, remotes with copy buttons, raw JSON toggleinputSchema/outputSchemaand annotationsGET /aliases/latestwith pinned/fallback color codingVersion detail tabs:
runtimeHint, andenvironmentVariableswithisRequired/isSecretbadges. Remotes shown as compact rows with transport tag + URL + copy button. "Show more / Show less" for long lists. Rawserver.jsontoggle with copy button at the bottom.inputSchema/outputSchemaas formatted JSON, and annotation badges. Raw tools JSON toggle with copy button.Screen.Recording.2026-06-12.at.7.24.36.PM.mov
Relates to RHOAIENG-65775, RHOAIENG-65774, RHOAIENG-65780, RHOAIENG-67491
Upstream / Downstream Impact
mlflow/mlflowUpstream backend PRs: MCP registry REST API, MCP registry REST client
Testing
8 test suites, 106 tests total - all passing. Manual testing with persistent SQLite backend.
Related Issues/PRs
Relates to RHOAIENG-65775, RHOAIENG-65774, RHOAIENG-65780, RHOAIENG-67491
Structured
server.jsonrendering (newServerJSONSectioncomponent):JSON.stringifyblock with structured UI sectionsruntimeHint, andenvironmentVariables(show more/less for >5 items)server.json: toggle button with chevron and copy buttonStructured tools rendering (new
ToolsSectioncomponent):inputSchema/outputSchemarendered as formatted JSON blocks with copy buttonsTabbed version detail layout:
Tabscomponent (Configuration, Tools, Access Bindings)server_jsonis immutable after creation)Version management:
server.jsonvalidation, status selection, source URL, tools JSON, and inline tag editordraft→active,active→draft/deprecated,deprecated→active)Latest version resolution:
GET /{name}/aliases/latest(pinned or fallback to most recent active)Compare view:
server.jsondiff with word-level highlightingServer-level features:
Context:
How is this PR tested?
Tests cover: package/remote rendering, expandable package rows with env vars, raw JSON toggle, tab switching, access bindings, version selection, compare view, status transitions, description display (read-only), display name editing, server-level tags, create modals.
Does this PR require documentation update?
Does this PR require updating the MLflow Skills repository?
Release Notes
Is this a user-facing change?
What component(s), interfaces, languages, and integrations does this PR affect?
Components
area/tracking: Tracking Service, tracking client APIs, autologgingarea/models: MLmodel format, model serialization/deserialization, flavorsarea/model-registry: Model Registry service, APIs, and the fluent client calls for Model Registryarea/scoring: MLflow Model server, model deployment tools, Spark UDFsarea/evaluation: MLflow model evaluation features, evaluation metrics, and evaluation workflowsarea/gateway: MLflow AI Gateway client APIs, server, and third-party integrationsarea/prompts: MLflow prompt engineering features, prompt templates, and prompt managementarea/tracing: MLflow Tracing features, tracing APIs, and LLM tracing functionalityarea/projects: MLproject format, project running backendsarea/uiux: Front-end, user experience, plotting, JavaScript, JavaScript dev serverarea/build: Build and test infrastructure for MLflowarea/docs: MLflow documentation pagesHow should the PR be classified in the release notes? Choose one:
rn/none- No description will be included. The PR will be mentioned only by the PR number in the "Small Bugfixes and Documentation Updates" sectionrn/breaking-change- The PR will be mentioned in the "Breaking Changes" sectionrn/feature- A new user-facing feature worth mentioning in the release notesrn/bug-fix- A user-facing bug fix worth mentioning in the release notesrn/documentation- A user-facing documentation change worth mentioning in the release notesIs this PR a critical bugfix or security fix that should go into the next patch release?